-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add gossip notifications / cluster node notifications to geyser #34618
Add gossip notifications / cluster node notifications to geyser #34618
Conversation
v1.17 is nearly at the end of its stabilization and only accepting PRs for bug fixes. Please consider opening this PR against master instead. |
I've used the 1.17 to have the grpc geyzer plugin compatible in version. I've created this PR first to do a test to validate the concept. It wasn't intended to be merged. If it seems to you interesting, I can move my change to the current master and try to adapt the grpc plugin to work with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the PR to against the master branch.
} | ||
}; | ||
//notify geyzer interface | ||
self.notify_clusterinfo_update(self.table.get(&gossip_label)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about for the upsert case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't understand. It's clusterinfo you prefer cluster_info?
use crate::crds_value::CrdsData; | ||
|
||
impl Crds { | ||
/// Notified when an account is updated at runtime, due to transaction activities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct these copy-pasted comments please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected. I change the comments.
@@ -533,6 +550,8 @@ impl Crds { | |||
self.shards.remove(index, &value); | |||
match value.value.data { | |||
CrdsData::LegacyContactInfo(_) => { | |||
//notify geyzer interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: --> geyser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
@@ -2,6 +2,8 @@ | |||
/// the GeyserPlugin trait to work with the runtime. | |||
/// In addition, the dynamic library must export a "C" function _create_plugin which | |||
/// creates the implementation of the plugin. | |||
use solana_sdk::pubkey::Pubkey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow our coding style, put them in "use {...}";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
|
||
/// Called when a cluster info is removed on gossip network. | ||
#[allow(unused_variables)] | ||
fn notify_clusterinfo_remove(&self, pubkey: &Pubkey) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this called? notify_clusterinfo_remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find the call here:
Line 554 in f4bc1be
self.notify_clusterinfo_remove(&value.value.pubkey()); |
I've created a new PR to start from master. I've issues with master rebase so I apply the commit to master branch directly and use a new branch. |
Problem
See issue #32934
Summary of Changes
This PR is a POC to add cluster node's info to the Geyzer plugin notification.
The LegacyContactInfo received on GOSSIP network are forwarded with the Geyzer interface to a client subscriber.
The PR contains:
A GRPC Yellowstone plugin update has been done to test the process and see the notified data volume. You can see the PR here.
An example of client can be found here. It subscribes to the Cluster info node notification and call from time to time the RPC get_cluster_nodes to compare with the notified data.
Early results
The test has been done on Testnet cluster with the Solana version 1.17.5 and Yellowstone plugin 1.11.0+solana.1.17.5.
Comparison with RPC call
It takes a few second for the plugin to get the majority of the Testnet cluster nodes and after 30s there's less than 10 nodes in error when comparing with the RPC call (around 3000 nodes in the cluster).
After a few minutes, Geyzer and RPC has the same list of cluster's nodes. From time to time they can have some difference (a few nodes) from one part or the other depending on the notification time propagation of update or removal.
Data volume
I use Tcpdump to monitor the packet send between the validator grpc server and the client to build the get_cluster_nodes RPC call nodes list.
This graphic is an example of network activity and bandwidth use. The volume of data transmitted is between 0,5 Mbits/s and 3 Mbits/s.
I've done some monitoring of the network connection between the grpc server and the client for the cluster node info notification.
This is the graphic of the data transferred between the validator grpc server and the client to build the get_cluster_nodes RPC call nodes list.